From 9cf705172fd6914185a4fbe1c969443942931784 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 19 Feb 2016 13:49:14 -0800 Subject: [PATCH] Merge the get/download methods on the Source trait Nothing currently implements the ability to more efficiently download a set of packages at any one point in time, and the download/get distinction isn't really used at all. We can always refactor later, but currently there's no benefit, nor can it really be seen what the possible benefit is, so let's just merge these two methods into one and have them operate on one id at a time. --- src/cargo/core/registry.rs | 25 ++++++------------ src/cargo/core/source.rs | 10 ++------ src/cargo/ops/cargo_install.rs | 5 ++-- src/cargo/sources/git/source.rs | 16 +++++------- src/cargo/sources/path.rs | 17 +++++-------- src/cargo/sources/registry.rs | 45 +++++++++++---------------------- 6 files changed, 39 insertions(+), 79 deletions(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index f56aaa93a..bd44d7787 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -2,7 +2,7 @@ use std::collections::{HashSet, HashMap}; use core::{Source, SourceId, SourceMap, Summary, Dependency, PackageId, Package}; use core::PackageSet; -use util::{CargoResult, ChainError, Config, human, profile}; +use util::{CargoResult, ChainError, Config, human, internal, profile}; /// Source of information about a group of packages. /// @@ -89,23 +89,14 @@ impl<'cfg> PackageRegistry<'cfg> { -> CargoResult> { trace!("getting packages; sources={}", self.sources.len()); - // TODO: Only call source with package ID if the package came from the - // source - let mut ret = Vec::new(); - - for (_, source) in self.sources.sources_mut() { - try!(source.download(package_ids)); - let packages = try!(source.get(package_ids)); - - ret.extend(packages.into_iter()); - } - - // TODO: Return earlier if fail - assert!(package_ids.len() == ret.len(), - "could not get packages from registry; ids={:?}; ret={:?}", - package_ids, ret); + let pkgs = try!(package_ids.iter().map(|id| { + let src = try!(self.sources.get_mut(id.source_id()).chain_error(|| { + internal(format!("failed to find a source listed for `{}`", id)) + })); + src.download(id) + }).collect::>>()); - Ok(PackageSet::new(ret, self.sources)) + Ok(PackageSet::new(pkgs, self.sources)) } fn ensure_loaded(&mut self, namespace: &SourceId, kind: Kind) -> CargoResult<()> { diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index 8cc679858..64779af37 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -9,7 +9,7 @@ use rustc_serialize::{Decodable, Decoder, Encodable, Encoder}; use url::Url; -use core::{Summary, Package, PackageId, Registry, Dependency}; +use core::{Package, PackageId, Registry}; use sources::{PathSource, GitSource, RegistrySource}; use sources::git; use util::{human, Config, CargoResult, ToUrl}; @@ -24,13 +24,7 @@ pub trait Source: Registry { /// The download method fetches the full package for each name and /// version specified. - fn download(&mut self, packages: &[PackageId]) -> CargoResult<()>; - - /// The get method returns the Path of each specified package on the - /// local file system. It assumes that `download` was already called, - /// and that the packages are already locally available on the file - /// system. - fn get(&self, packages: &[PackageId]) -> CargoResult>; + fn download(&mut self, package: &PackageId) -> CargoResult; /// Generates a unique string which represents the fingerprint of the /// current state of the source. diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 36fdabaf4..9bce345cb 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -130,9 +130,8 @@ fn select_pkg<'a, T>(mut source: T, let deps = try!(source.query(&dep)); match deps.iter().map(|p| p.package_id()).max() { Some(pkgid) => { - try!(source.download(&[pkgid.clone()])); - Ok((try!(source.get(&[pkgid.clone()])).remove(0), - Box::new(source))) + let pkg = try!(source.download(pkgid)); + Ok((pkg, Box::new(source))) } None => { let vers_info = vers.map(|v| format!(" with version `{}`", v)) diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 4a66b1c31..0cdb02f25 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -187,16 +187,12 @@ impl<'cfg> Source for GitSource<'cfg> { self.path_source.as_mut().unwrap().update() } - fn download(&mut self, _: &[PackageId]) -> CargoResult<()> { - // TODO: assert! that the PackageId is contained by the source - Ok(()) - } - - fn get(&self, ids: &[PackageId]) -> CargoResult> { - trace!("getting packages for package ids `{:?}` from `{:?}`", ids, - self.remote); - self.path_source.as_ref().expect("BUG: update() must be called \ - before get()").get(ids) + fn download(&mut self, id: &PackageId) -> CargoResult { + trace!("getting packages for package id `{}` from `{:?}`", id, + self.remote); + self.path_source.as_mut() + .expect("BUG: update() must be called before get()") + .download(id) } fn fingerprint(&self, _pkg: &Package) -> CargoResult { diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 206a9f0a4..26373c7ba 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -302,18 +302,13 @@ impl<'cfg> Source for PathSource<'cfg> { Ok(()) } - fn download(&mut self, _: &[PackageId]) -> CargoResult<()>{ - // TODO: assert! that the PackageId is contained by the source - Ok(()) - } - - fn get(&self, ids: &[PackageId]) -> CargoResult> { - trace!("getting packages; ids={:?}", ids); + fn download(&mut self, id: &PackageId) -> CargoResult { + trace!("getting packages; id={}", id); - Ok(self.packages.iter() - .filter(|pkg| ids.iter().any(|id| pkg.package_id() == id)) - .cloned() - .collect()) + let pkg = self.packages.iter().find(|pkg| pkg.package_id() == id); + pkg.cloned().ok_or_else(|| { + internal(format!("failed to find {} in path source", id)) + }) } fn fingerprint(&self, pkg: &Package) -> CargoResult { diff --git a/src/cargo/sources/registry.rs b/src/cargo/sources/registry.rs index 7e0ad0558..1c68475e0 100644 --- a/src/cargo/sources/registry.rs +++ b/src/cargo/sources/registry.rs @@ -187,7 +187,6 @@ pub struct RegistrySource<'cfg> { src_path: PathBuf, config: &'cfg Config, handle: Option, - sources: HashMap>, hashes: HashMap<(String, String), String>, // (name, vers) => cksum cache: HashMap>, updated: bool, @@ -239,7 +238,6 @@ impl<'cfg> RegistrySource<'cfg> { config: config, source_id: source_id.clone(), handle: None, - sources: HashMap::new(), hashes: HashMap::new(), cache: HashMap::new(), updated: false, @@ -537,37 +535,24 @@ impl<'cfg> Source for RegistrySource<'cfg> { Ok(()) } - fn download(&mut self, packages: &[PackageId]) -> CargoResult<()> { + fn download(&mut self, package: &PackageId) -> CargoResult { let config = try!(self.config()); let url = try!(config.dl.to_url().map_err(internal)); - for package in packages.iter() { - if self.source_id != *package.source_id() { continue } - if self.sources.contains_key(package) { continue } - - let mut url = url.clone(); - url.path_mut().unwrap().push(package.name().to_string()); - url.path_mut().unwrap().push(package.version().to_string()); - url.path_mut().unwrap().push("download".to_string()); - let path = try!(self.download_package(package, &url).chain_error(|| { - internal(format!("failed to download package `{}` from {}", - package, url)) - })); - let path = try!(self.unpack_package(package, path).chain_error(|| { - internal(format!("failed to unpack package `{}`", package)) - })); - let mut src = PathSource::new(&path, &self.source_id, self.config); - try!(src.update()); - self.sources.insert(package.clone(), src); - } - Ok(()) - } + let mut url = url.clone(); + url.path_mut().unwrap().push(package.name().to_string()); + url.path_mut().unwrap().push(package.version().to_string()); + url.path_mut().unwrap().push("download".to_string()); + let path = try!(self.download_package(package, &url).chain_error(|| { + internal(format!("failed to download package `{}` from {}", + package, url)) + })); + let path = try!(self.unpack_package(package, path).chain_error(|| { + internal(format!("failed to unpack package `{}`", package)) + })); - fn get(&self, packages: &[PackageId]) -> CargoResult> { - let mut ret = Vec::new(); - for src in self.sources.values() { - ret.extend(try!(src.get(packages)).into_iter()); - } - Ok(ret) + let mut src = PathSource::new(&path, &self.source_id, self.config); + try!(src.update()); + src.download(package) } fn fingerprint(&self, pkg: &Package) -> CargoResult { -- 2.30.2